STORM-2546: Fix storm-kafka-client spout getting stuck when retriable offsets were deleted from the Kafka log due to topic compaction#2307
Conversation
d7fdecb to
434bcbe
Compare
|
@askprasanna I've made the spout default to an auto offset reset policy of "earliest" when the user requests the at-least-once processing guarantee. I think this addresses your request here https://issues.apache.org/jira/browse/STORM-2546?focusedCommentId=16152824&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16152824. |
ed74307 to
95db9c0
Compare
|
@srdo Awesome. Thanks for following up. |
2b16ca6 to
6d486ac
Compare
HeartSaVioR
left a comment
There was a problem hiding this comment.
+1
My only 2 cents is that would we want to open the possibility to break the spout by theirselves (I mean users)?
If my understanding of this patch is right, both AT_LEAST_ONCE and other than earliest shouldn't be set. If I'm right we may want to restrict it with showing error message sooner, or at least expose warn message.
|
@srdo |
|
@HeartSaVioR Yes, you are right. We set the auto offset reset policy to The misconfiguration I'm most worried about is where users set I can't think of a reason why users would want to use those configurations, but I thought it might be better not to prevent the user from using those settings if they really want to, because there might be a use case I'm not seeing. I'm happy to add in checks and error messages (or maybe even throwing exceptions) when using those configurations, if you think it makes sense? |
|
@srdo |
|
@HeartSaVioR Added the warning. Will squash and merge if the warning looks good. |
HeartSaVioR
left a comment
There was a problem hiding this comment.
@srdo +1
Thanks for addressing quickly. Please go ahead squashing and merging.
Btw, could we have PR for 1.x branch as well?
… offsets were deleted from the Kafka log due to topic compaction
|
@HeartSaVioR Merged to master. Yes, I'll put up a 1.x version tonight. |
This requires the changes in #2156 for correctness, please ignore the first commit.
Here's the idea behind these changes https://issues.apache.org/jira/browse/STORM-2546?focusedCommentId=16151172&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16151172
KafkaSpoutCommitTest was renamed, so the first test in KafkaSpoutLogCompactionSupportTest was already there and can be skipped.